hotfix: with nodeAdapter we should get fetch from globalThis #1786
Conversation
|
| getSession: hooks.getSession || (() => ({})), | ||
| handle: hooks.handle || (({ request, resolve }) => resolve(request)), | ||
| serverFetch: hooks.serverFetch || fetch | ||
| serverFetch: hooks.serverFetch || globalThis.fetch |
There was a problem hiding this comment.
What about the fetch call inside serverFetch? Does that also need to be scoped with globalThis., or does this only affect the default case of not having serverFetch defined?
There was a problem hiding this comment.
no, it's fallback, serverFetch is optional.
There was a problem hiding this comment.
or does this only affect the default case of not having serverFetch defined?
yes.
This comment has been minimized.
This comment has been minimized.
|
I see globalThis.fetch = fetch2; on line ~3k of the output, but there's an init function on line ~1.7k that ends up calling fetch, so I guess they've gotten put out of order somehow It seems to be this init: kit/packages/kit/src/core/build/index.js Line 290 in 6ace879 I don't quite understand how all these build pieces fit together, but in my limited understanding this PR seems to be papering over the issue2, which is that |
|
I expect we may be hitting an incorrect assumption in esbuild: evanw/esbuild#399 (comment) |
|
Oof, that's a surprising issue. This does indeed seem to be an ordering issue — anything added to // foo.js
globalThis.foo = 42;// index.js
import './foo.js';
console.log(foo); // 42Is this something that broke recently? |
@Rich-Harris, yes, it started after d729b08#diff-ed48923f0655f935bee505cdcbf40212129ff49a89c2c9d752bd63aa1e6282c4R191 |
|
@benmccann, yes, I totally agree, it's just hiding some order issue, but at the same time, it can be a temporal solution while we are trying to find and fix it. |
|
I just noticed, but turns out this PR actually changes the dev environment, not the build one, so it's actually modifying the wrong files to fix the issue. Anyway, opened #1804 to try and deal with the root ordering issue rather than patch the symptoms. |
strange, at least I can't reproduce this issue anymore from #1784 after this PR. |
5 participants
Base on #1784 issue. Basically, under real nodejs we have no direct access to
fetcheven if it's set to globalThis.